T5 (#812): build-pipeline dual manifest + detached signatures#816
Open
GOKULRAJ136 wants to merge 3 commits into
Open
T5 (#812): build-pipeline dual manifest + detached signatures#816GOKULRAJ136 wants to merge 3 commits into
GOKULRAJ136 wants to merge 3 commits into
Conversation
WalkthroughAdds a detached-signature utility, extends manifest generation with a list-based mode, and updates the build script to generate, sign, package, and stage the new manifest and archive artifacts. ChangesDual-manifest detached signature pipeline
Sequence DiagramsequenceDiagram
participant configure as configure.sh
participant ManifestCreator
participant ManifestSigner
participant nginx as nginx static dir
configure->>ManifestCreator: folder mode → lib/MANIFEST.MF
configure->>ManifestSigner: sign lib/MANIFEST.MF → lib/MANIFEST.MF.sig
configure->>configure: zip JRE → jre21.zip
configure->>ManifestCreator: --list mode → root MANIFEST.MF
configure->>ManifestSigner: sign root MANIFEST.MF → MANIFEST.MF.sig
configure->>configure: zip lib/** → lib.zip
configure->>nginx: copy MANIFEST.MF.sig, lib.zip, jre21.zip
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@registration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.java`:
- Around line 60-62: The for loop iterating through libFolder.listFiles() at
line 60 (and again at line 91-96) processes all file system entries including
directories and non-regular files, but addEntryInManifest expects only regular
files that can be read as bytes, causing exceptions. Add a check using
File.isFile() before calling addEntryInManifest to filter out directories and
non-regular files, ensuring only regular files are processed. Apply this fix to
both locations mentioned in the comment.
- Around line 22-47: The catch block in the main method of ManifestCreator
currently logs the error from manifest creation but does not exit the process
with a failure code, allowing the application to terminate successfully even
when errors occur. After logging the error in the catch block that handles
Throwable exceptions (around line 44-45), add a System.exit call with a non-zero
exit code to ensure the process fails and prevents downstream packaging
operations from continuing with invalid or stale artifacts.
In
`@registration/registration-services/src/main/java/io/mosip/registration/update/ManifestSigner.java`:
- Around line 35-45: The main method currently accepts the keystore passphrase
as a command-line argument (args[1]), which exposes the secret in process
listings. Remove the storePass from the positional arguments and instead read it
from a protected source such as an environment variable, a secret file, or
standard input. Update the argument validation in the main method to reflect
that only 4 arguments are now required (keystorePath, alias, dataFile, sigFile),
then retrieve the passphrase from your chosen secure source and pass it to the
loadPrivateKey call instead of args[1].toCharArray().
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/update/ManifestSignerTest.java`:
- Around line 25-27: The static final constant STORE_PASS containing "changeit"
is a hardcoded credential that should not be committed to the repository. Remove
the STORE_PASS constant definition and instead generate a unique password
dynamically at runtime for each test execution. Modify the keytool invocation
(referenced at lines 112-113) to use the dynamically generated password instead
of the static STORE_PASS constant, ensuring no hardcoded secrets are propagated
through the test execution flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21896c00-c0f6-4826-b91a-63ef8d07f5e0
📒 Files selected for processing (5)
registration/configure.shregistration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.javaregistration/registration-services/src/main/java/io/mosip/registration/update/ManifestSigner.javaregistration/registration-services/src/test/java/io/mosip/registration/test/update/ManifestCreatorTest.javaregistration/registration-services/src/test/java/io/mosip/registration/test/update/ManifestSignerTest.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
registration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.java (1)
114-119: 🚀 Performance & Scalability | 🟠 MajorStream large files through
MessageDigestinstead of loading into memory.
addEntryInManifestloads the entire file into abyte[]viaFileUtils.readFileToByteArray(file). For the bundled lib jars this is tolerable, but the root manifest (configure.sh Line 144-149) invokes this method onjre21.zip— a full JRE archive of several hundred MB. Since the java process is launched without an-Xmxflag (configure.sh Line 144), this single-shot allocation can exhaust the container-constrained default heap and triggerOutOfMemoryError.
HMACUtils2.digestAsPlainText()does not provide a streaming variant, so implement hashing usingMessageDigestdirectly with a fixed-size buffer loop to read the file in chunks:private static void addEntryInManifest(Manifest manifest, File file) throws Exception { byte[] buffer = new byte[8192]; MessageDigest md = MessageDigest.getInstance("SHA-256"); try (FileInputStream fis = new FileInputStream(file)) { int bytesRead; while ((bytesRead = fis.read(buffer)) != -1) { md.update(buffer, 0, bytesRead); } } String hashText = HexFormat.of().formatHex(md.digest()); Attributes attribute = new Attributes(); attribute.put(Attributes.Name.CONTENT_TYPE, hashText); manifest.getEntries().put(file.getName(), attribute); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@registration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.java` around lines 114 - 119, The addEntryInManifest method loads entire files into memory using FileUtils.readFileToByteArray, which causes OutOfMemoryError when processing large files like jre21.zip. Replace the current implementation with streaming hash computation using MessageDigest directly: create a MessageDigest instance for SHA-256, open a FileInputStream with try-with-resources, read the file in fixed-size buffer chunks (8192 bytes), update the digest with each chunk, and finally convert the digest bytes to hex format using HexFormat. Remove the dependency on HMACUtils2.digestAsPlainText and FileUtils.readFileToByteArray.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@registration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.java`:
- Around line 114-119: The addEntryInManifest method loads entire files into
memory using FileUtils.readFileToByteArray, which causes OutOfMemoryError when
processing large files like jre21.zip. Replace the current implementation with
streaming hash computation using MessageDigest directly: create a MessageDigest
instance for SHA-256, open a FileInputStream with try-with-resources, read the
file in fixed-size buffer chunks (8192 bytes), update the digest with each
chunk, and finally convert the digest bytes to hex format using HexFormat.
Remove the dependency on HMACUtils2.digestAsPlainText and
FileUtils.readFileToByteArray.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36e57d3c-af6b-4237-9ad2-6d75bfaefc47
📒 Files selected for processing (4)
registration/configure.shregistration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.javaregistration/registration-services/src/main/java/io/mosip/registration/update/ManifestSigner.javaregistration/registration-services/src/test/java/io/mosip/registration/test/update/ManifestSignerTest.java
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@registration/configure.sh`:
- Around line 129-150: The signing step is using an unsafe classpath because
`java_cp` pulls in every jar from `target_dir/lib`, which may contain downloaded
or custom artifacts before `ManifestCreator` and `ManifestSigner` run. Update
the `configure.sh` flow so these two tool invocations use a minimal trusted
classpath with only the project’s own jars, or move them earlier before
`target_dir/lib` is populated, and keep the references to `java_cp`,
`ManifestCreator`, and `ManifestSigner` aligned with the trusted-only build
path.
In
`@registration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.java`:
- Around line 114-115: The manifest hashing in addEntryInManifest currently
reads the entire File into memory via FileUtils.readFileToByteArray, which can
create a build-memory hotspot for large artifacts like jre21.zip. Update this
helper to compute the HMACUtils2 digest by streaming the file contents from disk
instead of buffering the whole file, while keeping the manifest entry logic in
ManifestCreator unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c7b3dbc-8eec-4a67-b843-53561235bcb7
📒 Files selected for processing (5)
registration/configure.shregistration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.javaregistration/registration-services/src/main/java/io/mosip/registration/update/ManifestSigner.javaregistration/registration-services/src/test/java/io/mosip/registration/test/update/ManifestCreatorTest.javaregistration/registration-services/src/test/java/io/mosip/registration/test/update/ManifestSignerTest.java
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
registration/configure.sh (1)
151-167: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove old zip outputs before recreating release archives.
zip -rupdates an existing archive and can retain entries that were removed fromjre/orlib/in a rerun, so stale files may be served injre21.ziporlib.zip. Delete the target archives first.Proposed fix
# jre21.zip : the JRE artifact referenced by the root manifest / downloaded by the launcher cd "${target_dir}" +rm -f jre21.zip /usr/bin/zip -r jre21.zip jre ... # lib.zip : all of lib/** (including the signed lib/MANIFEST.MF) hosted from the upgrade server +rm -f lib.zip /usr/bin/zip -r lib.zip lib🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@registration/configure.sh` around lines 151 - 167, The release archive creation in configure.sh can retain stale entries because zip -r updates existing archives instead of recreating them. Before generating jre21.zip and lib.zip in the release packaging flow, remove any pre-existing target archives first so reruns don’t carry removed files forward; update the archive-building steps around the jre21.zip and lib.zip creation commands to always start from a clean slate.registration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.java (1)
82-89: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSkip existing manifest artifacts before hashing the lib folder.
When
configure.shcalls this with${target_dir}/libas both the scanned folder and manifest target, a rerun can pick up staleMANIFEST.MForMANIFEST.MF.sig, hash those old bytes, then overwrite them. That can produce a signed manifest whose entries no longer match the packaged files.Proposed fix
+ File canonicalTarget = targetFile.getCanonicalFile(); + String signatureFileName = targetFile.getName() + ".sig"; for (File file : children) { - if (file.isFile()) { - addEntryInManifest(manifest, file); - } else { + if (!file.isFile()) { logger.warn("Skipping non-regular file in manifest folder: {}", file.getName()); + continue; } + if (file.getCanonicalFile().equals(canonicalTarget) || signatureFileName.equals(file.getName())) { + logger.warn("Skipping generated manifest artifact in manifest folder: {}", file.getName()); + continue; + } + addEntryInManifest(manifest, file); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@registration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.java` around lines 82 - 89, Skip existing manifest artifacts before generating the manifest in ManifestCreator so reruns do not hash stale outputs. Update the file iteration in createManifest/addEntryInManifest flow to ignore MANIFEST.MF and MANIFEST.MF.sig when scanning the lib folder, especially when the target directory is also the source directory. Keep the existing logger.warn behavior for other non-regular files, and ensure write(manifest, targetFile) only runs after filtering these artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@registration/configure.sh`:
- Around line 151-167: The release archive creation in configure.sh can retain
stale entries because zip -r updates existing archives instead of recreating
them. Before generating jre21.zip and lib.zip in the release packaging flow,
remove any pre-existing target archives first so reruns don’t carry removed
files forward; update the archive-building steps around the jre21.zip and
lib.zip creation commands to always start from a clean slate.
In
`@registration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.java`:
- Around line 82-89: Skip existing manifest artifacts before generating the
manifest in ManifestCreator so reruns do not hash stale outputs. Update the file
iteration in createManifest/addEntryInManifest flow to ignore MANIFEST.MF and
MANIFEST.MF.sig when scanning the lib folder, especially when the target
directory is also the source directory. Keep the existing logger.warn behavior
for other non-regular files, and ensure write(manifest, targetFile) only runs
after filtering these artifacts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 066d56fc-d2f1-4978-a476-f981b7426f27
📒 Files selected for processing (3)
registration/configure.shregistration/registration-services/src/main/java/io/mosip/registration/update/ManifestCreator.javaregistration/registration-services/src/test/java/io/mosip/registration/test/update/ManifestCreatorTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refer Story #807
Fixes #812
Summary by CodeRabbit
Release Notes
New Features
MANIFEST.MFsignatures, packagedlibcontents, and a bundled JRE archive for deployment.Bug Fixes
Tests